web sourced footage, illustration engines, and layout qc for explainers - #148
Open
DonIsmaelito wants to merge 24 commits into
Open
DonIsmaelito wants to merge 24 commits into
DonIsmaelito wants to merge 24 commits into
Conversation
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
28 issues found across 40 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="helpers/web_source.py">
<violation number="1" location="helpers/web_source.py:107">
P1: A DNS hostname resolving to a private or loopback address passes this public-URL check, allowing yt-dlp to fetch internal endpoints. Resolve all host addresses before downloading, reject non-global results, and apply the same protection across redirects.</violation>
<violation number="2" location="helpers/web_source.py:321">
P2: Distinct source IDs can alias to one download folder because `slug` lowercases and truncates the identity. Add a hash of the exact extractor and source ID to the folder name so one source cannot overwrite another source's artifacts.</violation>
<violation number="3" location="helpers/web_source.py:790">
P2: Sub-millisecond source ranges collapse to the same inspection tag and asset ID, allowing selection of an interval different from the visually inspected one. Reject inputs beyond the supported precision or use one lossless canonical range encoding for tags, IDs, and command arguments.</violation>
<violation number="4" location="helpers/web_source.py:844">
P2: A cached `source.*` file can make `acquire()` return without creating or repairing `acquisition.json`, leaving the downloaded source without acquisition provenance. Write the manifest for cached files too, or require a valid existing manifest before returning.</violation>
</file>
<file name="helpers/render.py">
<violation number="1" location="helpers/render.py:896">
P2: When captions come from SRT, the default rail does not contain the rendered text. `SUB_FORCE_STYLE` places captions around y=0.69, while this rail starts at y=0.84, so split and center overlays can cover subtitle words. Align the safe region with the actual subtitle style or derive both from one shared setting.</violation>
<violation number="2" location="helpers/render.py:1152">
P2: Invalid overlay contracts are checked only after segment extraction and concat, so failed renders still spend the full staging cost. Validate each selected overlay set before starting extraction.</violation>
<violation number="3" location="helpers/render.py:1368">
P2: When an overlay has an unknown `fit`, preflight treats it as `cover` but final compositing rejects it. Validate `fit` identically in preflight so the approval gate cannot pass an EDL that the render cannot process.</violation>
<violation number="4" location="helpers/render.py:1677">
P2: When a deliverable has custom overlays or a different aspect ratio, `--preflight-overlays` checks the wrong inputs. It uses the global overlay list and unreframed base, while `render_one_output` renders the deliverable-specific list on a reframed base. Run preflight for each selected deliverable using the same base and overlays as its render.</violation>
<violation number="5" location="helpers/render.py:1693">
P2: When two deliverables declare the same output file, `--all-deliverables` silently overwrites the first result with the second. Reject duplicate resolved output paths before starting the render.</violation>
</file>
<file name="helpers/captions.py">
<violation number="1" location="helpers/captions.py:28">
P2: Invalid finite-range timestamps are accepted and can crash subtitle generation or emit zero-duration events. Reject non-finite and negative times in both word and character alignment normalization before chunking.</violation>
<violation number="2" location="helpers/captions.py:167">
P2: Two-line captions can protrude above the declared safe rail, especially at the supported minimum `safe_bottom` or smaller output heights. Validate that the rail can contain the configured font and wrapped cue, or reduce the font/wrapping before writing the ASS style.</violation>
</file>
<file name="tests/test_skill_contract.py">
<violation number="1" location="tests/test_skill_contract.py:55">
P2: Deleting a hard rule that is not in HARD_RULES goes undetected. SKILL.md currently has 14 hard rules but HARD_RULES only lists 12 (rules 13 'Captions transcribe audible speech only.' and 14 'Generated layouts are collision-free at every critical frame.' are missing). The numbering assertion only verifies consecutiveness, and the per-index loop only covers HARD_RULES entries, so deleting rule 13 or 14 and renumbering keeps numbers consecutive and passes (verified: removing rule 14 keeps both checks green). This defeats the stated purpose of catching a hard rule that 'silently disappears'. Add rules 13-14 to HARD_RULES and tighten the length check to equality.</violation>
<violation number="2" location="tests/test_skill_contract.py:58">
P3: The 'changed or moved' check uses assertIn substring containment, so a rule can be silently reworded or weakened while still passing (e.g. 'Never cut inside a word.' becoming 'Never cut inside a word when possible'). Only full removal or renumbering is reliably caught. Consider matching the rule-name phrase more strictly if drift detection is intended.</violation>
</file>
<file name="helpers/layout_qc.py">
<violation number="1" location="helpers/layout_qc.py:59">
P3: `_number` silently coerces booleans: `float(True)` is `1.0` and `float(False)` is `0.0`, so a JSON manifest field such as `"width": true` or `"x": false` passes validation instead of being rejected. For a QC tool whose purpose is catching layout mistakes, a misspelled/typed boolean in a numeric field gets silently accepted as 1/0 pixels. Reject `bool` explicitly before the float conversion.</violation>
<violation number="2" location="helpers/layout_qc.py:111">
P3: `validate_frame` never validates `time`, so a direct caller passing a non-numeric `time` (e.g. a string, as the manifest docs encourage calling `validate_frame` directly per frame) raises a raw `ValueError` from the `{time:.3f}` format instead of `LayoutQCError`. That breaks the module's documented contract ("error type raised for any layout problem so callers can catch one thing"). `validate_manifest` avoids this only because it pre-validates `time` with `_number`; `validate_frame` should too.</violation>
<violation number="3" location="helpers/layout_qc.py:136">
P2: When valid fractional rectangle coordinates land exactly on a canvas edge, binary floating-point addition can make `rect.right` or `rect.bottom` slightly larger than the canvas and reject the layout. Compare edge values with a small floating-point tolerance before reporting an out-of-canvas element.</violation>
<violation number="4" location="helpers/layout_qc.py:174">
P2: A manifest with a negative frame time currently passes QC even though it cannot identify a real video frame. Reject negative times after parsing the frame time.</violation>
<violation number="5" location="helpers/layout_qc.py:188">
P3: When the manifest is missing, unreadable, malformed, or fails validation, `main` exposes a traceback instead of a concise CLI error. Catch the input and `LayoutQCError` exceptions and exit with their message.</violation>
</file>
<file name="references/web-sourcing.md">
<violation number="1" location="references/web-sourcing.md:102">
P2: This doc tells the agent to read `skills/manim-video/references/concept-explainer.md` before authoring a Manim scene, but that file does not exist anywhere in the repo. Following the procedure will fail at the read step, and the promised eyebrow-text / fit-and-overlap guidance is unavailable. Either add the file in this PR or point to an existing reference such as `skills/manim-video/references/visual-design.md` or `scene-planning.md` that actually provides the checks described.</violation>
</file>
<file name="references/overlays.md">
<violation number="1" location="references/overlays.md:49">
P3: The protected-region rule is documented as applying only to split and picture-in-picture overlays, but the implementation rejects any overlay with a named layout or custom rect (only `cutaway` and legacy rect-less overlays are exempt). An agent following this doc could author a `center`-layout overlay over an illustration expecting it to pass, then hit a validation error. Widen the wording to match the code.</violation>
</file>
<file name="pyproject.toml">
<violation number="1" location="pyproject.toml:25">
P3: This PR adds a [tool.pytest.ini_options] section and test files that `import pytest` and `from helpers.*`, but pytest is declared nowhere in pyproject.toml (runtime deps and the `animations` extra are the only groups). A fresh checkout cannot run the test suite, and the `pythonpath` option only exists in pytest >= 7.0. Add a dev/test extra, e.g. `[project.optional-dependencies] test = ["pytest>=7"]`, so the tests this config enables are actually runnable.</violation>
</file>
<file name="helpers/edl.py">
<violation number="1" location="helpers/edl.py:103">
P2: When version 2 declares `subtitles` as an empty string without `captions`, this early return bypasses the required non-empty-path validation. Check for `subtitles is None` instead so an explicitly empty subtitle path is rejected.</violation>
<violation number="2" location="helpers/edl.py:258">
P2: A deliverable that sets `reframe_track` but has no `reframe` block (neither on the deliverable nor shared at the EDL root) silently becomes a `cover` crop and the named track is dropped. `reframe_track` is only honored when `raw is not None`, so with no mode declared anywhere the code falls through to `{"mode": "cover"}` and ignores the requested track. This is a silent downgrade that contradicts the module's own guarantee that a tracked request never silently falls back to a center crop. Default the mode to `track` when `reframe_track` is set and no mode is resolved, or raise an error, instead of defaulting to `cover`.</violation>
<violation number="3" location="helpers/edl.py:351">
P2: When `sample_rate_hz` is fractional, `int()` truncates it to 48000 and the validator accepts the wrong requested rate. Reject non-integral sample rates before the integer comparison.</violation>
</file>
<file name="helpers/render_illustration.py">
<violation number="1" location="helpers/render_illustration.py:53">
P2: When an unrelated or older `roger` is on `PATH`, `ensure_roger` skips the pinned 3.3.1 install and renders with an uncontrolled CLI version. Always use the versioned cache, or verify the PATH executable is exactly 3.3.1 before returning it.</violation>
<violation number="2" location="helpers/render_illustration.py:138">
P2: When either renderer is called directly with a nested relative path, changing `cwd` makes the source path resolve twice and can place the output in the wrong directory. Resolve source, output, cache, and dump paths inside the renderer functions instead of relying on `main()`.</violation>
</file>
<file name="tests/test_captions_substation.py">
<violation number="1" location="tests/test_captions_substation.py:16">
P3: The suite doesn't cover load_words's fallback and failure handling: normalized_alignment (the key ElevenLabs actually returns, and the first key load_words checks) is never tested, and neither is the ValueError raised when alignment array lengths mismatch, nor the dropping of entries without valid timing, nor the end<=start clamping in _as_word. Add tests for these so regressions in the helper's primary path and error handling are caught.</violation>
</file>
<file name="tests/test_layout_qc.py">
<violation number="1" location="tests/test_layout_qc.py:53">
P3: The test name claims it checks canvas bounds at each critical frame, but both frames' elements are well inside the 1080x1920 canvas, so the out-of-bounds path is never exercised. Rename it to reflect what it actually verifies (frame/element counts for a multi-frame manifest) or add an element that leaves the canvas to make the name accurate.</violation>
</file>
Tip: instead of fixing issues one by one fix them all with cubic
Re-trigger cubic
| if parsed.scheme not in {"http", "https"} or not parsed.netloc: | ||
| raise ValueError("source URL must be a public http or https URL") | ||
| hostname = parsed.hostname or "" | ||
| if hostname.casefold() == "localhost" or hostname.endswith(".localhost"): |
There was a problem hiding this comment.
P1: A DNS hostname resolving to a private or loopback address passes this public-URL check, allowing yt-dlp to fetch internal endpoints. Resolve all host addresses before downloading, reject non-global results, and apply the same protection across redirects.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At helpers/web_source.py, line 107:
<comment>A DNS hostname resolving to a private or loopback address passes this public-URL check, allowing yt-dlp to fetch internal endpoints. Resolve all host addresses before downloading, reject non-global results, and apply the same protection across redirects.</comment>
<file context>
@@ -0,0 +1,1182 @@
+ if parsed.scheme not in {"http", "https"} or not parsed.netloc:
+ raise ValueError("source URL must be a public http or https URL")
+ hostname = parsed.hostname or ""
+ if hostname.casefold() == "localhost" or hostname.endswith(".localhost"):
+ raise ValueError("local URLs are not valid public sources")
+ # when the host is a literal ip reject anything that is not globally routable
</file context>
| [tool.setuptools] | ||
| py-modules = [] | ||
|
|
||
| [tool.pytest.ini_options] |
There was a problem hiding this comment.
P3: This PR adds a [tool.pytest.ini_options] section and test files that import pytest and from helpers.*, but pytest is declared nowhere in pyproject.toml (runtime deps and the animations extra are the only groups). A fresh checkout cannot run the test suite, and the pythonpath option only exists in pytest >= 7.0. Add a dev/test extra, e.g. [project.optional-dependencies] test = ["pytest>=7"], so the tests this config enables are actually runnable.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At pyproject.toml, line 25:
<comment>This PR adds a [tool.pytest.ini_options] section and test files that `import pytest` and `from helpers.*`, but pytest is declared nowhere in pyproject.toml (runtime deps and the `animations` extra are the only groups). A fresh checkout cannot run the test suite, and the `pythonpath` option only exists in pytest >= 7.0. Add a dev/test extra, e.g. `[project.optional-dependencies] test = ["pytest>=7"]`, so the tests this config enables are actually runnable.</comment>
<file context>
@@ -21,3 +21,6 @@ build-backend = "setuptools.build_meta"
[tool.setuptools]
py-modules = []
+
+[tool.pytest.ini_options]
+pythonpath = ["."]
</file context>
| self.assertEqual(numbers, list(range(1, len(numbers) + 1)), "rules must be numbered consecutively") | ||
| self.assertGreaterEqual(len(items), len(HARD_RULES), "a hard rule was removed") | ||
| for index, expected in enumerate(HARD_RULES): | ||
| self.assertIn(expected, items[index][1], f"rule {index + 1} changed or moved") |
There was a problem hiding this comment.
P3: The 'changed or moved' check uses assertIn substring containment, so a rule can be silently reworded or weakened while still passing (e.g. 'Never cut inside a word.' becoming 'Never cut inside a word when possible'). Only full removal or renumbering is reliably caught. Consider matching the rule-name phrase more strictly if drift detection is intended.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At tests/test_skill_contract.py, line 58:
<comment>The 'changed or moved' check uses assertIn substring containment, so a rule can be silently reworded or weakened while still passing (e.g. 'Never cut inside a word.' becoming 'Never cut inside a word when possible'). Only full removal or renumbering is reliably caught. Consider matching the rule-name phrase more strictly if drift detection is intended.</comment>
<file context>
@@ -0,0 +1,71 @@
+ self.assertEqual(numbers, list(range(1, len(numbers) + 1)), "rules must be numbered consecutively")
+ self.assertGreaterEqual(len(items), len(HARD_RULES), "a hard rule was removed")
+ for index, expected in enumerate(HARD_RULES):
+ self.assertIn(expected, items[index][1], f"rule {index + 1} changed or moved")
+
+ # every backticked helper or reference path in skill md must exist on disk
</file context>
| # tests for load_words input handling | ||
| class LoadWordsTests(unittest.TestCase): | ||
| # elevenlabs character timings are merged into words with the right start and end | ||
| def test_reads_elevenlabs_character_alignment(self): |
There was a problem hiding this comment.
P3: The suite doesn't cover load_words's fallback and failure handling: normalized_alignment (the key ElevenLabs actually returns, and the first key load_words checks) is never tested, and neither is the ValueError raised when alignment array lengths mismatch, nor the dropping of entries without valid timing, nor the end<=start clamping in _as_word. Add tests for these so regressions in the helper's primary path and error handling are caught.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At tests/test_captions_substation.py, line 16:
<comment>The suite doesn't cover load_words's fallback and failure handling: normalized_alignment (the key ElevenLabs actually returns, and the first key load_words checks) is never tested, and neither is the ValueError raised when alignment array lengths mismatch, nor the dropping of entries without valid timing, nor the end<=start clamping in _as_word. Add tests for these so regressions in the helper's primary path and error handling are caught.</comment>
<file context>
@@ -0,0 +1,80 @@
+# tests for load_words input handling
+class LoadWordsTests(unittest.TestCase):
+ # elevenlabs character timings are merged into words with the right start and end
+ def test_reads_elevenlabs_character_alignment(self):
+ payload = {
+ "alignment": {
</file context>
| # coerce a value to a finite float or raise a labeled qc error | ||
| def _number(value: Any, label: str) -> float: | ||
| try: | ||
| number = float(value) |
There was a problem hiding this comment.
P3: _number silently coerces booleans: float(True) is 1.0 and float(False) is 0.0, so a JSON manifest field such as "width": true or "x": false passes validation instead of being rejected. For a QC tool whose purpose is catching layout mistakes, a misspelled/typed boolean in a numeric field gets silently accepted as 1/0 pixels. Reject bool explicitly before the float conversion.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At helpers/layout_qc.py, line 59:
<comment>`_number` silently coerces booleans: `float(True)` is `1.0` and `float(False)` is `0.0`, so a JSON manifest field such as `"width": true` or `"x": false` passes validation instead of being rejected. For a QC tool whose purpose is catching layout mistakes, a misspelled/typed boolean in a numeric field gets silently accepted as 1/0 pixels. Reject `bool` explicitly before the float conversion.</comment>
<file context>
@@ -0,0 +1,194 @@
+# coerce a value to a finite float or raise a labeled qc error
+def _number(value: Any, label: str) -> float:
+ try:
+ number = float(value)
+ except (TypeError, ValueError) as exc:
+ raise LayoutQCError(f"{label} must be numeric") from exc
</file context>
Suggested change
| number = float(value) | |
| if isinstance(value, bool): | |
| raise LayoutQCError(f"{label} must be numeric") | |
| number = float(value) |
|
|
||
|
|
||
| # a manifest with two frames returns frame and element counts | ||
| def test_manifest_checks_bounds_at_each_critical_frame() -> None: |
There was a problem hiding this comment.
P3: The test name claims it checks canvas bounds at each critical frame, but both frames' elements are well inside the 1080x1920 canvas, so the out-of-bounds path is never exercised. Rename it to reflect what it actually verifies (frame/element counts for a multi-frame manifest) or add an element that leaves the canvas to make the name accurate.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At tests/test_layout_qc.py, line 53:
<comment>The test name claims it checks canvas bounds at each critical frame, but both frames' elements are well inside the 1080x1920 canvas, so the out-of-bounds path is never exercised. Rename it to reflect what it actually verifies (frame/element counts for a multi-frame manifest) or add an element that leaves the canvas to make the name accurate.</comment>
<file context>
@@ -0,0 +1,78 @@
+
+
+# a manifest with two frames returns frame and element counts
+def test_manifest_checks_bounds_at_each_critical_frame() -> None:
+ payload = {
+ "canvas": {"width": 1080, "height": 1920},
</file context>
…ames in the skill contract
DonIsmaelito
force-pushed
the
feature/explainer-visual-tools
branch
from
September 2, 2026 23:31
114ba58 to
0c60a59
Compare
…nd guard hard rule thirteen
…al hosts and guard hard rule fourteen
…audio events in captions and tidy docs
DonIsmaelito
force-pushed
the
feature/explainer-visual-tools
branch
from
September 2, 2026 23:44
0c60a59 to
9fecb2f
Compare
There was a problem hiding this comment.
9 existing issues remain and 23 new issues found across 35 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="README.md">
<violation number="1" location="README.md:24">
P2: The normal `render.py` path never invokes `layout_qc.py`, so collisions do not fail a render as this bullet promises. State the explicit QC command and workflow requirement, or wire layout validation into render preflight.</violation>
</file>
<file name="references/layout-qc.md">
<violation number="1" location="references/layout-qc.md:29">
P3: The manifest example does not demonstrate `allow_overlap_with`: it names a `button` that is absent, and the cursor does not overlap the title or any other element. Add an overlapping button element so this example actually validates the documented intentional-intersection contract.</violation>
</file>
<file name="references/deliverables.md">
<violation number="1" location="references/deliverables.md:10">
P2: This example is not a renderable `edl.json` as shown: copying it into the documented render commands fails because required `sources` and `ranges` are omitted. Label the block as a declaration fragment or include the required EDL fields so the documented commands can be followed.</violation>
</file>
<file name="references/overlays.md">
<violation number="1" location="references/overlays.md:35">
P3: This rule contradicts the documented and implemented `picture_in_picture` custom-rect form, so authors cannot tell which EDL shape is valid. Restrict the prohibition to `cutaway` and `split_*`, while stating the PiP exception.</violation>
<violation number="2" location="references/overlays.md:57">
P2: For a custom non-full-width `captions.safe_region`, `full` is not shortened as this section promises; the renderer rejects it when the rectangles intersect. Document the full-width requirement or define how a full layout should avoid a partial rail.</violation>
</file>
<file name="install.md">
<violation number="1" location="install.md:62">
P2: The macOS install block installs Typst unconditionally when it is missing, contradicting the stated lazy-install behavior and the `optional, CeTZ only` comment. Defer this command to the first CeTZ render or make it an explicit opt-in step.</violation>
</file>
<file name="tests/test_layout_qc.py">
<violation number="1" location="tests/test_layout_qc.py:44">
P3: The test only covers the case where the element the overlap is declared on comes first in the element list. `helpers/layout_qc.py` line 143 accepts the declaration on either element (`second_id in first_allowed or first_id in second_allowed`), so a regression that drops the `first_id in second_allowed` branch would pass the current suite. Add a case where only the later element carries `allow_overlap_with`.</violation>
</file>
<file name="references/web-sourcing.md">
<violation number="1" location="references/web-sourcing.md:68">
P1: Following step 8 literally can produce an EDL that fails web-overlay validation because it omits the required `id` or `beat_id`. Carry the selection's `beat_id` into the overlay, typically as its `id`, alongside the provenance fields.</violation>
<violation number="2" location="references/web-sourcing.md:98">
P1: Narrated EDL v2 projects still fail validation after following this caption step because the required `captions.provenance` evidence is not declared. Tell authors to list the narration alignment JSON under `captions.provenance.files` with `kind: narration_alignment` before converting it to ASS.</violation>
</file>
<file name="helpers/render_illustration.py">
<violation number="1" location="helpers/render_illustration.py:61">
P2: When two projects render Penrose for the first time concurrently, both processes can pass this existence check and run `npm install` into the same cache prefix. Serialize cache initialization with an inter-process lock, or install into a temporary prefix and rename it only after success.</violation>
</file>
<file name="tests/test_comment_style.py">
<violation number="1" location="tests/test_comment_style.py:15">
P2: Comments containing `!`, `?`, or other punctuation pass this check, so the test does not enforce its stated no-punctuation convention. Match the full punctuation set instead.</violation>
<violation number="2" location="tests/test_comment_style.py:31">
P3: A hash-comment header after six leading blank lines bypasses this header check because `lines[:6]` excludes it, and the AST docstring check does not see comments. Scan all leading lines until the first non-shebang nonempty line.</violation>
</file>
<file name="helpers/edl.py">
<violation number="1" location="helpers/edl.py:519">
P2: A non-empty tracked list is accepted without validating its keyframe fields. Validate each keyframe's numeric, increasing, normalized fields during `validate_edl` so malformed tracking data fails before expensive rendering.</violation>
</file>
<file name="helpers/web_source.py">
<violation number="1" location="helpers/web_source.py:106">
P1: When a source URL contains username/password userinfo, this validator accepts it and metadata persists the secret. Reject URLs with `parsed.username` or `parsed.password` before returning the URL.</violation>
<violation number="2" location="helpers/web_source.py:121">
P2: When DNS resolution fails or the URL has no hostname, the public-host check fails open and passes the URL to yt-dlp. Reject an empty resolution result instead of treating it as public.</violation>
<violation number="3" location="helpers/web_source.py:343">
P2: When yt-dlp returns no `id`, a second metadata fetch can choose a different folder because this fallback hashes volatile fields, causing `select` to reject a window that was already inspected. Derive the folder fallback from the same stable URL identity used by `source_key`.</violation>
<violation number="4" location="helpers/web_source.py:991">
P3: When `inspect` is run with `--start/--end` on a video without captions, the requested window inspection is silently skipped: the function returns at the caption gate before downloading the proxy or rendering the filmstrip, and a subsequent `select` fails with "source window has not been visually inspected". The filmstrip doesn't need captions, so either perform the window inspection when `source_range` is given or fail explicitly that the requested window was not inspected.</violation>
<violation number="5" location="helpers/web_source.py:1043">
P2: When `acquire` receives `--start/--end`, it does not force exact section cuts, unlike the proxy path, so `source_<range>` may not match the approved interval. Add `--force-keyframes-at-cuts` or cut the high-quality download explicitly with ffmpeg.</violation>
<violation number="6" location="helpers/web_source.py:1082">
P1: When a kept selection still has the default `rights_status=needs-review`, `acquire` accepts it and downloads the source. Require an explicit rights-cleared approval state before allowing acquisition.</violation>
</file>
<file name="SKILL.md">
<violation number="1" location="SKILL.md:195">
P3: This change puts a new subtitle procedure directly in `SKILL.md`, contrary to the repository rule that procedure prose belongs in feature references. Move the workflow to a reference file and leave only a one-line pointer here.</violation>
</file>
<file name="helpers/layout_qc.py">
<violation number="1" location="helpers/layout_qc.py:59">
P2: A manifest containing an integer outside float range raises raw `OverflowError` instead of `LayoutQCError`. Catch `OverflowError` alongside `TypeError` and `ValueError` in `_number()`.</violation>
<violation number="2" location="helpers/layout_qc.py:122">
P3: Non-string element IDs are silently coerced, while `allow_overlap_with` requires string IDs; notably ID `0` is treated as missing. Reject non-string IDs instead of converting them with `str(...)`.</violation>
</file>
<file name="helpers/render.py">
<violation number="1" location="helpers/render.py:1641">
P3: The `--preflight-base` branch decides `has_subtitles` from the raw EDL field (`not args.no_subtitles and bool(edl.get("subtitles"))`) without checking that the subtitle file exists, while the post-concat preflight branch uses `subs_path is not None and subs_path.exists()` after resolving and dropping missing files. When the EDL points at a missing subtitle file, the fast preflight still reserves/marks the caption rail and enforces the rail-collision errors even though the real composite would burn no subtitles, so the contact sheet and the final composite disagree.</violation>
</file>
Requires human review: Auto-approval blocked because this review re-detected 10 unresolved issues already reported by Cubic.
Tip: instead of fixing issues one by one fix them all with cubic
Re-trigger cubic
Comment on lines
+98
to
+100
| Narrated original explainers require captions. Generate timestamped narration, | ||
| convert the timestamps with `helpers/captions.py`, and reserve the bottom 16% of | ||
| the frame as `captions.safe_region`. Captions may not cover sourced footage or |
There was a problem hiding this comment.
P1: Narrated EDL v2 projects still fail validation after following this caption step because the required captions.provenance evidence is not declared. Tell authors to list the narration alignment JSON under captions.provenance.files with kind: narration_alignment before converting it to ASS.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At references/web-sourcing.md, line 98:
<comment>Narrated EDL v2 projects still fail validation after following this caption step because the required `captions.provenance` evidence is not declared. Tell authors to list the narration alignment JSON under `captions.provenance.files` with `kind: narration_alignment` before converting it to ASS.</comment>
<file context>
@@ -0,0 +1,117 @@
+file: measure the assembled track before loudness normalization and fail or
+repair the edit when it has no finite loudness or no audible samples.
+
+Narrated original explainers require captions. Generate timestamped narration,
+convert the timestamps with `helpers/captions.py`, and reserve the bottom 16% of
+the frame as `captions.safe_region`. Captions may not cover sourced footage or
</file context>
Suggested change
| Narrated original explainers require captions. Generate timestamped narration, | |
| convert the timestamps with `helpers/captions.py`, and reserve the bottom 16% of | |
| the frame as `captions.safe_region`. Captions may not cover sourced footage or | |
| Narrated original explainers require captions. Generate timestamped narration and list its JSON alignment under `captions.provenance.files` with `kind: narration_alignment`, | |
| convert the timestamps with `helpers/captions.py`, and reserve the bottom 16% of | |
| the frame as `captions.safe_region`. |
Comment on lines
+68
to
+70
| in the existing EDL `overlays` list. Keep narration audio primary. Set | ||
| `media_kind: web` and retain `source_url`, `source_start`, `source_end`, and | ||
| `asset_id` from the selection manifest. |
There was a problem hiding this comment.
P1: Following step 8 literally can produce an EDL that fails web-overlay validation because it omits the required id or beat_id. Carry the selection's beat_id into the overlay, typically as its id, alongside the provenance fields.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At references/web-sourcing.md, line 68:
<comment>Following step 8 literally can produce an EDL that fails web-overlay validation because it omits the required `id` or `beat_id`. Carry the selection's `beat_id` into the overlay, typically as its `id`, alongside the provenance fields.</comment>
<file context>
@@ -0,0 +1,117 @@
+ another valid basis to use it. Preserve attribution for reusable licensed work.
+8. Cut, crop, scale, and grade the selected interval with ffmpeg into an
+ output-sized silent clip under `<edit>/overlays/`, then place that prepared file
+ in the existing EDL `overlays` list. Keep narration audio primary. Set
+ `media_kind: web` and retain `source_url`, `source_start`, `source_end`, and
+ `asset_id` from the selection manifest.
</file context>
Suggested change
| in the existing EDL `overlays` list. Keep narration audio primary. Set | |
| `media_kind: web` and retain `source_url`, `source_start`, `source_end`, and | |
| `asset_id` from the selection manifest. | |
| in the existing EDL `overlays` list. Keep narration audio primary. Set | |
| `media_kind: web`, use the selection's `beat_id` as the overlay `id`, and retain | |
| `source_url`, `source_start`, `source_end`, and `asset_id` from the selection manifest. |
| kept = [ | ||
| item | ||
| for item in manifest.get("selections", []) | ||
| if item.get("decision") == "keep" and item.get("source_key") == source_id |
There was a problem hiding this comment.
P1: When a kept selection still has the default rights_status=needs-review, acquire accepts it and downloads the source. Require an explicit rights-cleared approval state before allowing acquisition.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At helpers/web_source.py, line 1082:
<comment>When a kept selection still has the default `rights_status=needs-review`, `acquire` accepts it and downloads the source. Require an explicit rights-cleared approval state before allowing acquisition.</comment>
<file context>
@@ -0,0 +1,1226 @@
+ kept = [
+ item
+ for item in manifest.get("selections", [])
+ if item.get("decision") == "keep" and item.get("source_key") == source_id
+ ]
+ if source_range is not None:
</file context>
| # accept only http or https urls that point at public hosts | ||
| def validate_public_url(value: str) -> str: | ||
| parsed = urlparse(value) | ||
| if parsed.scheme not in {"http", "https"} or not parsed.netloc: |
There was a problem hiding this comment.
P1: When a source URL contains username/password userinfo, this validator accepts it and metadata persists the secret. Reject URLs with parsed.username or parsed.password before returning the URL.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At helpers/web_source.py, line 106:
<comment>When a source URL contains username/password userinfo, this validator accepts it and metadata persists the secret. Reject URLs with `parsed.username` or `parsed.password` before returning the URL.</comment>
<file context>
@@ -0,0 +1,1226 @@
+# accept only http or https urls that point at public hosts
+def validate_public_url(value: str) -> str:
+ parsed = urlparse(value)
+ if parsed.scheme not in {"http", "https"} or not parsed.netloc:
+ raise ValueError("source URL must be a public http or https URL")
+ hostname = parsed.hostname or ""
</file context>
Suggested change
| if parsed.scheme not in {"http", "https"} or not parsed.netloc: | |
| if ( | |
| parsed.scheme not in {"http", "https"} | |
| or not parsed.netloc | |
| or parsed.username is not None | |
| or parsed.password is not None | |
| ): |
| - **Persists session memory** in `project.md` so next week's session picks up where you left off | ||
| - **Renders every delivery format from one edit** — 16:9 and 9:16 with per-format loudness targets and keyframed reframing, validated before any render starts | ||
| - **Finds real footage for explainers** — searches public video, inspects exact moments, records provenance, and places footage in split or picture-in-picture compositions that never cover captions | ||
| - **Checks generated layouts** — text and component collisions fail the render before you see it |
There was a problem hiding this comment.
P2: The normal render.py path never invokes layout_qc.py, so collisions do not fail a render as this bullet promises. State the explicit QC command and workflow requirement, or wire layout validation into render preflight.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At README.md, line 24:
<comment>The normal `render.py` path never invokes `layout_qc.py`, so collisions do not fail a render as this bullet promises. State the explicit QC command and workflow requirement, or wire layout validation into render preflight.</comment>
<file context>
@@ -16,9 +16,12 @@ Try video-use in [Browser Use Cloud](https://cloud.browser-use.com/v4?utm_campai
- **Persists session memory** in `project.md` so next week's session picks up where you left off
+- **Renders every delivery format from one edit** — 16:9 and 9:16 with per-format loudness targets and keyframed reframing, validated before any render starts
+- **Finds real footage for explainers** — searches public video, inspects exact moments, records provenance, and places footage in split or picture-in-picture compositions that never cover captions
+- **Checks generated layouts** — text and component collisions fail the render before you see it
## Setup prompt
</file context>
Suggested change
| - **Checks generated layouts** — text and component collisions fail the render before you see it | |
| - **Checks generated layouts** — text and component collisions fail the render before you see it | |
| + **Checks generated layouts** — run `layout_qc.py` on generated layout manifests to reject text and component collisions before review |
| lines = text.splitlines() | ||
| problems = [] | ||
| # the file opens with a docstring and not with a block of hash comments | ||
| first = [line for line in lines[:6] if line.strip() and not line.startswith("#!")] |
There was a problem hiding this comment.
P3: A hash-comment header after six leading blank lines bypasses this header check because lines[:6] excludes it, and the AST docstring check does not see comments. Scan all leading lines until the first non-shebang nonempty line.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At tests/test_comment_style.py, line 31:
<comment>A hash-comment header after six leading blank lines bypasses this header check because `lines[:6]` excludes it, and the AST docstring check does not see comments. Scan all leading lines until the first non-shebang nonempty line.</comment>
<file context>
@@ -0,0 +1,83 @@
+ lines = text.splitlines()
+ problems = []
+ # the file opens with a docstring and not with a block of hash comments
+ first = [line for line in lines[:6] if line.strip() and not line.startswith("#!")]
+ if first and first[0].startswith("#"):
+ problems.append("hash comment header should be a module docstring")
</file context>
|
|
||
| ## Subtitles (when requested) | ||
|
|
||
| First verify that the final audio contains audible speech and that timestamped |
There was a problem hiding this comment.
P3: This change puts a new subtitle procedure directly in SKILL.md, contrary to the repository rule that procedure prose belongs in feature references. Move the workflow to a reference file and leave only a one-line pointer here.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At SKILL.md, line 195:
<comment>This change puts a new subtitle procedure directly in `SKILL.md`, contrary to the repository rule that procedure prose belongs in feature references. Move the workflow to a reference file and leave only a one-line pointer here.</comment>
<file context>
@@ -177,6 +192,15 @@ Hard rules: apply **per-segment during extraction** (not post-concat, which re-e
## Subtitles (when requested)
+First verify that the final audio contains audible speech and that timestamped
+transcript or alignment JSON exists. Subtitle text must be derived from those
+spoken words. Never invent caption sentences to summarize a music-only video.
</file context>
| if not isinstance(raw, dict): | ||
| problems.append(f"{label} must be an object{suffix}") | ||
| continue | ||
| element_id = str(raw.get("id") or "").strip() |
There was a problem hiding this comment.
P3: Non-string element IDs are silently coerced, while allow_overlap_with requires string IDs; notably ID 0 is treated as missing. Reject non-string IDs instead of converting them with str(...).
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At helpers/layout_qc.py, line 122:
<comment>Non-string element IDs are silently coerced, while `allow_overlap_with` requires string IDs; notably ID `0` is treated as missing. Reject non-string IDs instead of converting them with `str(...)`.</comment>
<file context>
@@ -0,0 +1,194 @@
+ if not isinstance(raw, dict):
+ problems.append(f"{label} must be an object{suffix}")
+ continue
+ element_id = str(raw.get("id") or "").strip()
+ if not element_id:
+ problems.append(f"{label} requires an id{suffix}")
</file context>
| print(f"saved candidate metadata and sidecars: {folder}") | ||
|
|
||
| caption_file = choose_caption_file(folder) | ||
| if caption_file is None: |
There was a problem hiding this comment.
P3: When inspect is run with --start/--end on a video without captions, the requested window inspection is silently skipped: the function returns at the caption gate before downloading the proxy or rendering the filmstrip, and a subsequent select fails with "source window has not been visually inspected". The filmstrip doesn't need captions, so either perform the window inspection when source_range is given or fail explicitly that the requested window was not inspected.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At helpers/web_source.py, line 991:
<comment>When `inspect` is run with `--start/--end` on a video without captions, the requested window inspection is silently skipped: the function returns at the caption gate before downloading the proxy or rendering the filmstrip, and a subsequent `select` fails with "source window has not been visually inspected". The filmstrip doesn't need captions, so either perform the window inspection when `source_range` is given or fail explicitly that the requested window was not inspected.</comment>
<file context>
@@ -0,0 +1,1226 @@
+ print(f"saved candidate metadata and sidecars: {folder}")
+
+ caption_file = choose_caption_file(folder)
+ if caption_file is None:
+ update_transcript_status(folder, "skip-no-transcript")
+ print("SKIP: no captions were available; move to the next candidate")
</file context>
| sys.exit(f"preflight base not found: {base_path}") | ||
| if out_path is None: | ||
| ap.error("overlay preflight requires -o/--output") | ||
| has_subtitles = not args.no_subtitles and bool(edl.get("subtitles")) |
There was a problem hiding this comment.
P3: The --preflight-base branch decides has_subtitles from the raw EDL field (not args.no_subtitles and bool(edl.get("subtitles"))) without checking that the subtitle file exists, while the post-concat preflight branch uses subs_path is not None and subs_path.exists() after resolving and dropping missing files. When the EDL points at a missing subtitle file, the fast preflight still reserves/marks the caption rail and enforces the rail-collision errors even though the real composite would burn no subtitles, so the contact sheet and the final composite disagree.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At helpers/render.py, line 1641:
<comment>The `--preflight-base` branch decides `has_subtitles` from the raw EDL field (`not args.no_subtitles and bool(edl.get("subtitles"))`) without checking that the subtitle file exists, while the post-concat preflight branch uses `subs_path is not None and subs_path.exists()` after resolving and dropping missing files. When the EDL points at a missing subtitle file, the fast preflight still reserves/marks the caption rail and enforces the rail-collision errors even though the real composite would burn no subtitles, so the contact sheet and the final composite disagree.</comment>
<file context>
@@ -721,7 +1595,60 @@ def main() -> None:
+ sys.exit(f"preflight base not found: {base_path}")
+ if out_path is None:
+ ap.error("overlay preflight requires -o/--output")
+ has_subtitles = not args.no_subtitles and bool(edl.get("subtitles"))
+ build_overlay_preflight(
+ base_path,
</file context>
This was referenced Sep 17, 2026
Author
|
@cubic-dev-ai please review the latest commits again. Inherited the delivery and caption protection fixes from #147. Validation: 69 branch tests passed. The combined core preview passed 765 tests with two optional skips. Existing threads remain open for rechecking; this follow-up does not claim every previous finding is resolved. |
@DonIsmaelito There are no new changes to review since the last completed review. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
Explainer videos sometimes need footage or diagrams that the supplied recordings do not contain. These helpers acquire source footage with provenance, render diagrams and check visual layouts.
Changes
Limits
Builds on #147 and references teaching guidance from #146. Diagram engines require their optional external tools. Users supply their own Penrose specifications or appropriately licensed material; the helpers do not establish reuse rights or mathematical correctness.